Skip to content

Conversation

@y21
Copy link
Member

@y21 y21 commented Oct 3, 2024

Fixes #10619

Just ran into this myself and I definitely agree it's not very nice to have to manually go through all the types involved to figure out why this happens and to evaluate if this is really a problem (knowing if the field of a struct is something that a hash impl relies on), so this changes the lint to emit notes for each step involved.

changelog: none

@rustbot
Copy link
Collaborator

rustbot commented Oct 3, 2024

r? @Alexendoo

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Oct 3, 2024
span_lint_and_then(cx, MUTABLE_KEY_TYPE, span, "mutable key type", |diag| {
for ty in chain.iter().rev() {
diag.note(with_forced_trimmed_paths!(format!(
"... because it contains `{ty}`, which has interior mutability"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since type names can be quite long sometimes, wouldn't it be best to use instead of ... here? In monospace font the difference would be two characters wide:

... because it contains `UnsafeCell<usize>`, which has interior mutability
… because it contains `UnsafeCell<usize>`, which has interior mutability

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went with the three dots because I think it's much more common in diagnostics (in fact, I think I've never actually seen a non-ASCII character like that in a diagnostic - rust-lang/rust#126597 adds unicode block drawing but it requires an extra --error-format=human-unicode).

Some rustc examples
Some cargo examples

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a "…" in a diagnostic issued by clippy_lints/src/methods/or_then_unwrap.rs:

clippy_lints/src/methods/or_then_unwrap.rs
25:        title = "found `.or(Some(…)).unwrap()`";
32:        title = "found `.or(Ok(…)).unwrap()`";

@Alexendoo
Copy link
Member

👍

... is fine with me, but if we decide to use we can change all of them at once

@bors r+

@bors
Copy link
Contributor

bors commented Oct 7, 2024

📌 Commit 19d1358 has been approved by Alexendoo

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 7, 2024

⌛ Testing commit 19d1358 with merge b013e69...

@bors
Copy link
Contributor

bors commented Oct 7, 2024

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Alexendoo
Pushing b013e69 to master...

@bors bors merged commit b013e69 into rust-lang:master Oct 7, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mutable_key_type doesn't mention the type which makes it confusing

5 participants